Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Allow for configuration of foldlevel similar to Emacs Orgmode #615

Merged

Conversation

PriceHiller
Copy link
Contributor

@PriceHiller PriceHiller commented Sep 25, 2023

Summary

This PR permits configuration of the default fold level via the setup variable org_startup_folded.

  • This PR fixes the issues identified in Set Default Fold Level #614 as a user can now better modify the fold level used by Orgmode
  • This PR also solves issues with nvim-ufo integration (Compatibility with nvim-ufo #599) to a certain degree
    • It "solves" the issue for nvim-ufo users (such as I) by permitting them to inherit their vim.opt.foldlevel instead of using Orgmode's provided fold level. This makes Orgmode play nice with nvim-ufo. My earlier problems with fillchars cannot be reproduced at this time, so I consider it a non-issue.
  • Wrote documentation to the DOCS.md file for the new option. As a note I have not written new documentation to the Orgmode help file. I noticed that it appeared autogenerated so I held off on that until I get some confirmation.

Notes

  • This PR introduced the new configuration variable as org_startup_folded with a default value of overview.
  • The org_startup_folded mirrors the values and usage from Emacs equivalent with the same values for configuration
  • This new configuration option doesn't play nice with users who lazy load on the Filetype event as the foldlevel is set upon that event matching for org file types.
    • A way to fix that is to add a line in their config vim.cmd.edit() after Neorg's initial load which will reload the file and retrigger the Filetype event permitting the event to work as anticipated.
  • I had to pass the Orgmode config around in the lua/orgmode/init.lua file such that I could access the value of config.org_startup_folded and set the foldlevel accordingly.
  • This PR conditionally sets the foldlevel value with logic embedded directly into the setup_autocmds FileType autocmd. If you dislike that, please do guide me as to where that logic should be placed.
  • I created an inline type for the org_startup_folded variable in the DefaultConfig. This seems to be the only place within the DefaultConfig where that is done. If this is not desired I can readily rebase and remove the annotation.

A Recommendation

If Nvim Orgmode isn't deadset on being a 100% spec accurate implementation of Orgmode (which I suspect it is attempting to be), I would recommend rejecting this PR in favor of using a more Vim-centric way of setting the foldlevel.

That is, don't even set the foldlevel, make the user set it themselves. If they want a different foldlevel for only org files then they can use an autocmd or a ftplugin located in their config at after/ftplugin/org.lua.

On the other hand, if Nvim Orgmode is deadset on being a 100% compliant implementation of Orgmode, then this PR will bring the implementation closer to the spec.

Fin.

Feel free to provide critique and advice, I'm certainly inexperienced in working with Nvim Orgmode's code base, this PR marks the first time I have ever done so. Any thoughts you have are very much desired 😃.

Closes #614
Only partially resolves #599, for a tighter integration we may want to autodetect nvim-ufo and set the foldlevel accordingly.

@PriceHiller
Copy link
Contributor Author

PriceHiller commented Sep 25, 2023

As a note it appears github will try to autoclose #599 on the acceptance of this PR, I recommend against that unless you believe this is a sufficient solution. A tighter integration may be desired, read the last bit of the PR for an idea 😉.

@orhnk
Copy link

orhnk commented Sep 25, 2023

Why not just have a depth variable?

With this approach you can:

require("orgmode").setup {
    fold = {
        -- depth = 1, --> custom depth
        depth = vim.o.foldlevel, --> respect the options
        -- Other fold options ... --
    }
}

@PriceHiller
Copy link
Contributor Author

Sure, that'd work. I was trying to be compliant with Emacs orgmode, I'm unsure as to how much Nvim Orgmode intends to mirror it.

At that point though, why even support a depth variable? We get that for free via vim.opt.foldlevel if they set it and a user can use after/ftplugin/org.lua if they need a custom depth for only org files or an autocmd.

Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Code looks ok, but I would like it to live on the config class and called in the ftplugin/org.vim.

elseif config.org_startup_folded == 'showeverything' then
vim.opt_local.foldlevel = 99
elseif config.org_startup_folded == 'inherit' then
vim.opt_local.foldlevel = vim.opt.foldlevel:get()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this. It should inherit it automatically. We just need to not do anything in case of inherit value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point! I will remove that. I'll get to this in a few hours, have a few things I gotta knock out today.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done as part of the refactor to place this in the config class. Take a look.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not change anything in this file but instead introduce a method on the config called setup_foldlevel() and call it in the ftplugin/org.vim as we do for the mappings here

lua require('orgmode.config'):setup_mappings('text_objects')

lua require('orgmode.config'):setup_foldlevel()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do, will get to this in a few hours when I get a sec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, see the config class now.

Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's much cleaner now, thanks!

We just need to fix the fallback to be set properly since it won't be correctly set.

vim.opt_local.foldlevel = 99
elseif self.org_startup_folded ~= 'inherit' then
vim.notify("Invalid option passed for 'org_startup_folded'!", vim.log.levels.WARN)
self.org_startup_folded = 'overview'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since config does not have __newindex metamethod, this will not be set in the correct place.
It should be self.opts.org_startup_folded = 'overview'.

For the warning message, please use utils.echo_warning since that's what we use for other warnings so it is consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, done now. Did not know that Orgmode had its own utilities for info/warn/error.

Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@kristijanhusak kristijanhusak merged commit 5325a8e into nvim-orgmode:master Sep 26, 2023
4 checks passed
@PriceHiller
Copy link
Contributor Author

@kristijanhusak

Hey would you like me to go mention this in the ufo issue thread? If you set the setting to inherit or showeverything this resolves the issues I had with ufo not playing nice. Seems to work fine now.

@kristijanhusak
Copy link
Member

Sure, go ahead.

@PriceHiller PriceHiller deleted the feat/foldlevel-configuration branch September 26, 2023 13:24
SlayerOfTheBad pushed a commit to SlayerOfTheBad/orgmode that referenced this pull request Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set Default Fold Level Compatibility with nvim-ufo
3 participants